Fix length of non-beat clips in the pattern editor#8369
Conversation
I think this was an oversight, there is no reason for melody clips to behave differently than beat clips in the same pattern.
|
Is this likely to break backward compatibility with projects that have a different step size in pattern editor than its contents? I ask because I probably have projects like that. |
Sure ! Maybe my explanations are confusing 😅. I will capture a video when able to, to show what I mean. |
You mean if you have clips inside a pattern that are longer than the amount of steps in the pattern ? Then yes, this would break your project and you would have to manually add steps to the pattern. |
Like promised, I have updated the description with a before / after demonstration. |
|
I think this looks pretty cool! |
|
Glad this has been addressed! |
| auto mClip = static_cast<MidiClip*>(track->getClip(m_ps->currentPattern())); | ||
| m_maxClipLength = std::max(m_maxClipLength, static_cast<tick_t>(mClip->length())); | ||
| } | ||
| else |
There was a problem hiding this comment.
This else covers every track type that isn't Instrument, which btw...
enum class Type
{
Instrument,
Pattern,
Sample,
Event,
Video,
Automation,
HiddenAutomation,
Count
} ;
Cursed, cursed, cursed!
@messmerd is it a safe assumption that this else only catches Sample and Automation?
There was a problem hiding this comment.
I completely missed the fact that there were so many types of tracks 😅 (What even is a Video or Count track?)
I guess it would indeed do no harm to be explicit here, as we only only resize Sample and Automation clips for visual purposes (and all these other types, even if they happened to be present in the pattern store, are never displayed anyway).
But in the end, it doesn't change much.
There was a problem hiding this comment.
Yeah, maybe an else if for samples and automation, and an else assert(false) since we're not supposed to reach that for now.
There was a problem hiding this comment.
You could also use a switch statement:
switch (track->type())
{
case Track::Type::Instrument:
{
auto mClip = static_cast<MidiClip*>(track->getClip(m_ps->currentPattern()));
m_maxClipLength = std::max(m_maxClipLength, static_cast<tick_t>(mClip->length()));
break;
}
case Track::Type::Sample: [[fallthrough]];
case Track::Type::Automation:
{
auto clip = track->getClip(m_ps->currentPattern());
clip->updateLength();
break;
}
default:
assert(false);
break;
}There was a problem hiding this comment.
Should the m_maxClipLength variable be updated for sample and automation tracks too?
There was a problem hiding this comment.
Well... actually yes...
I'm a little embarrassed by my self from 2 weeks ago. (I believe I somehow thought that it had an impact on the pattern length ?!)
(Edit: I fact my past me just left this block as it was previously, which is pretty reasonable)
|
|
||
| SampleClip::SampleClip(Track* _track, Sample sample, bool isPlaying) | ||
| : Clip(_track) | ||
| , m_sampleTrack(_track) |
There was a problem hiding this comment.
Clip already stores a Track, and has a getter called getTrack().
There was a problem hiding this comment.
That's the first thing I should have checked. Thank you for pointing it out.
| auto mClip = static_cast<MidiClip*>(track->getClip(m_ps->currentPattern())); | ||
| m_maxClipLength = std::max(m_maxClipLength, static_cast<tick_t>(mClip->length())); | ||
| } | ||
| else |
There was a problem hiding this comment.
You could also use a switch statement:
switch (track->type())
{
case Track::Type::Instrument:
{
auto mClip = static_cast<MidiClip*>(track->getClip(m_ps->currentPattern()));
m_maxClipLength = std::max(m_maxClipLength, static_cast<tick_t>(mClip->length()));
break;
}
case Track::Type::Sample: [[fallthrough]];
case Track::Type::Automation:
{
auto clip = track->getClip(m_ps->currentPattern());
clip->updateLength();
break;
}
default:
assert(false);
break;
}
messmerd
changed the title
bratpeki
left a comment
bratpeki
left a comment
There was a problem hiding this comment.
Approving for code and testing.
5 participants
This PR is correcting the following:
Before:
2026-05-04.17-43-10.mp4
After:
2026-05-04.17-45-30.mp4
To my knowledge, no currently opened issue would be resolved by this PR, but I could be wrong.